Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: make SourceMap static + module import improvements #960

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

feds01
Copy link
Contributor

@feds01 feds01 commented Sep 5, 2023

  • ast+expand: implement #dump_ast directive
  • ast: move tree.rs into hash-ast-utils
  • move AST pretty printing from hash-fmt into hash-ast-utils
  • ast-utils: change module descriptio
  • source: make SourceMap static
  • reporting: rename compiler_note! to note_on_span!
  • tir: remove &Path from mod-def due to source map being accessible
  • ast: simplify ImportExpr by storing import SourceId over resolved path
  • pipeline: ensure resolved module imports always canonicalise paths

Todo

@feds01 feds01 self-assigned this Sep 5, 2023
@feds01 feds01 requested a review from kontheocharis September 5, 2023 20:29
@kontheocharis
Copy link
Collaborator

I wonder if NodeMap should be given the same fate..

@feds01
Copy link
Contributor Author

feds01 commented Sep 8, 2023

I wonder if NodeMap should be given the same fate...

I think it depends on what we want to do with it. It isn't as widely used across the compiler as the SourceMap - Parser, expand, desugar, untyped-semantic, and analysis- But they all mostly use it at the top level so... is it really worth making it static?

Not strictly related but:

  • given that the AST doesn't need to live beyond TIR, we could drop it.
  • In the future, we will likely have to make it support concurrent accesses (in order to allow for things like macros to modify it.

@feds01 feds01 requested a review from kontheocharis September 8, 2023 18:23
@kontheocharis
Copy link
Collaborator

A couple of points:

  1. TIR stores are static, which contain AstNodeIds, which refer to things inside the NodeMap. This might be an indicator that NodeMap should be static.
  2. If the AST does not need to live after TIR then it can still be dropped by clearing the static data. In fact that should happen with the TIR too.

@kontheocharis
Copy link
Collaborator

This doesn't necessarily mean it should be static; if we make it static we are just moving the issue forward and can use the same logic to argue that the workspace should be static too. So maybe we could stop at the source map. Or we could make the whole workspace static. I think anything in between doesn't make much sense..

@feds01
Copy link
Contributor Author

feds01 commented Sep 8, 2023

My dream is to make the workspace a kind of "user" facing API which enables you to essentially configure a compiler to do the kind of work you want it to do, e.g. a workspace that builds an executable, or a library, a test suite, etc. For example, you could in a build script do:

main := () => {
   // Create a main workspace.
    w := Compiler::create_workspace();
    w.set_entry_point("src/main.hash");
    w.set_exe_output("out/main");
    w.build();

   // Create a testing workspace.
    w := Compiler::create_workspace();
    w.set_entry_point("tests/main.hash");
    w.set_exe_output("out/main-tests");
    w.build();
}

Perhaps it would make sense to detach NodeMap and SourceMap from the workspace since they now become shared across the entire compiler session?

@kontheocharis
Copy link
Collaborator

I think to have that interface, at this point, this API would need to separately invoke hashc in a subprocess. Otherwise the stuff we have static already is problematic..

@feds01
Copy link
Contributor Author

feds01 commented Sep 8, 2023

Yeah the workspace would effectively spawn a new compiler instance, and run separately from the other workspaces and perhaps the build environment too.

@kontheocharis
Copy link
Collaborator

kontheocharis commented Sep 8, 2023

In that case, surely the workspace itself could be made static too. (I mean the Workspace struct in hashc)

@feds01
Copy link
Contributor Author

feds01 commented Sep 8, 2023

Yeah it probably would mean to make the workspace static too.

@feds01
Copy link
Contributor Author

feds01 commented Sep 8, 2023

I think this can be done later though, it's not a need-to-do right now task I think.

@feds01 feds01 merged commit 1329ecd into main Sep 8, 2023
1 check passed
@feds01 feds01 deleted the static-sources branch September 8, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants